Skip to content

Introduce PreparedOperation #82

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 22 commits into from
Closed

Conversation

mp911de
Copy link
Member

@mp911de mp911de commented Mar 22, 2019

We now encapsulate prepared operations from the StatementFactory within PreparedOperation that renders SQL and provides binding values.

StatementFactory supports SELECT/INSERT/UPDATE/DELETE statement creation considering Dialect-specific rendering.
StatementFactory replaces String-based statement methods in ReactiveDataAccessStrategy.

PreparedOperation<Update> operation = accessStrategy.getStatements().update(entity.getTableName(), binder -> {
	binder.bind("name", "updated value");
	binder.filterBy("id", SettableValue.from(42));
});

databaseClient.execute().sql(operation).then();

Related ticket: #73.

mp911de added a commit that referenced this pull request Apr 2, 2019
We now consider BigDecimal and BigInteger as simple types and no longer as entities.

We also no longer set properties whose value is null.
@mp911de mp911de requested a review from schauder April 11, 2019 07:15
mp911de added 3 commits April 18, 2019 10:06
We now encapsulate prepared operations from the StatementFactory within PreparedOperation that renders SQL and provides binding values.

StatementFactory supports SELECT/INSERT/UPDATE/DELETE statement creation considering Dialect-specific rendering.
StatementFactory replaces String-based statement methods in ReactiveDataAccessStrategy.

PreparedOperation<Update> operation = accessStrategy.getStatements().update(entity.getTableName(), binder -> {
	binder.bind("name", "updated value");
	binder.filterBy("id", SettableValue.from(42));
});

databaseClient.execute().sql(operation).then();
Switch license header URLs to HTTPS. Rebase onto master.
@mp911de mp911de force-pushed the issue/73-prepared-operation branch from 4f72724 to 8dce8ad Compare April 18, 2019 08:19
Copy link
Contributor

@schauder schauder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The relationship between (Prepared)Operation and Statement and their respective relationship to SQL Strings confuses me and I think we should improve and clarify that.

A PreparedOperation encapsulates/creates a SQL String.
But its bind method gets a Statement as an argument which also encapsulates a SQL String.

Sometimes it seems you are suffering from the same confusion, since the StatementFactory doesn't produces Statement instances but PreparedOperation instances.

I'm wondering if we could/should implement the whole thing in a way that references to a Statement appear only in the inside of an Operation.

/**
* Binder to specify parameter bindings by name. Bindings match to equals comparisons.
*/
interface StatementBinderBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of that name. I made me look for a build method returning a StatementBinder and didn't help me otherwise.

How about a SomethingCustomizer or CustomizableSomething. Something might actually be Query, Statement or Operation. Again a point in the case that we need to clarify these terms.

@mp911de
Copy link
Member Author

mp911de commented Apr 18, 2019

Yeah, let's discuss the underlying concept first and then find good names for our classes.

@schauder
Copy link
Contributor

With these changes the Statement doesn't leave the PreparedOperation anymore. What do you think?

}

@Override
public Statement bind(Connection connection) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bind(…) is somewhat misleading now. That would be rather createBoundStatement(). There is now no hook that would allow us to modify the SQL that is used to create the Statement.

Right now, without #46, all this makes little sense.

Query Pre-processing would look like:

QueryFilterFunction filter = ofPreparedOperation(preparedOperation -> {

  PreparedOperation operation = PreparedOperation.from(preparedOperation)
  .filterQuery(sql -> sql + " AND tenantId = :tenant")
  .withBinding("tenantId", "foobar")
  .build();

  return operation;
});

filter.and(ofPreparedOperation(operation -> {
  AUDIT_LOGGER.info("Executed Query: " + operation.toQuery());
  return operation;
}))

@schauder schauder force-pushed the issue/73-prepared-operation branch from 6d0bc57 to 2265c7f Compare May 6, 2019 09:00
mp911de added a commit that referenced this pull request May 6, 2019
We now encapsulate prepared operations from the StatementFactory within PreparedOperation that renders SQL and provides binding values.

StatementFactory supports SELECT/INSERT/UPDATE/DELETE statement creation considering Dialect-specific rendering.
StatementFactory replaces String-based statement methods in ReactiveDataAccessStrategy.

PreparedOperation<Update> operation = accessStrategy.getStatements().update(entity.getTableName(), binder -> {
	binder.bind("name", "updated value");
	binder.filterBy("id", SettableValue.from(42));
});

databaseClient.execute().sql(operation).then();

Original pull request: #82.
mp911de pushed a commit that referenced this pull request May 6, 2019
Original pull request: #82.
mp911de added a commit that referenced this pull request May 6, 2019
We now apply bindings to a BindTarget that can be overridden without the need to implement all Statement methods.

PreparedOperation no longer has a direct dependency on R2DBC Statement.

Original pull request: #82.
@mp911de mp911de closed this May 6, 2019
@mp911de mp911de deleted the issue/73-prepared-operation branch May 6, 2019 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants